Create external memory only snapshot #6512
Conversation
| if not vm.is_alive(): | ||
| vm.start() | ||
|
|
||
| def _all_disk_targets(): |
There was a problem hiding this comment.
This function is useful to get all devs in the vm xml.
Could you please add this function into avocado-vt/virttest/utils_libvirt/libvirt_disk.py?
But please also refine your codes.
There was a problem hiding this comment.
Please directly use avocado-framework/avocado-vt#4231 instead of duplicating the codes here.
BulaYoungR
force-pushed
the
snapshot_memonly
branch
2 times, most recently
from
918c0c4 to
a046c52
Compare
BulaYoungR
force-pushed
the
snapshot_memonly
branch
2 times, most recently
from
02c1d7e to
ccaf6c6
Compare
| scenario = cli_diskspec | ||
| - xml_snapshot_no: | ||
| scenario = xml_snapshot_no | ||
| set_snapshot_no_in_xml = yes |
There was a problem hiding this comment.
Please remove this parameter
BulaYoungR
force-pushed
the
snapshot_memonly
branch
from
ccaf6c6 to
757114a
Compare
WalkthroughAdds a new Avocado-VT memory-only snapshot test and config: Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Tester
participant Runner as run(test, params, env)
participant VM
participant Virsh
Tester->>Runner: invoke run(...)
rect rgba(220,235,255,0.25)
note right of Runner: setup_test
Runner->>VM: validate main_vm & fetch domain XML
Runner->>VM: collect disk targets
Runner->>VM: ensure VM is running
Runner->>Runner: determine mem_file & scenario
alt xml_diskspec
Runner->>VM: modify XML (set disk snapshot='no')
else cli_diskspec
Runner->>Runner: build --diskspec args per disk
end
end
rect rgba(220,255,220,0.25)
note right of Runner: _snapshot_create
Runner->>Virsh: snapshot-create-as --no-metadata --memspec=<mem> --live [--diskspec...]
Virsh-->>Runner: command result
end
rect rgba(255,240,220,0.25)
note right of Runner: teardown_test (finally)
Runner->>VM: restore original XML if modified
end
Runner-->>Tester: return result
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
libvirt/tests/cfg/snapshot/virsh_memonly_snapshot.cfg (2)
3-3: Remove redundant start_vm; the test manages VM state.Keeping this can leave the VM running before XML edits, breaking the xml_snapshot_no path. Also matches prior feedback.
- start_vm = yes
1-1: Top-level key must not be a list item.Drop the leading dash; otherwise the cfg may not parse as expected.
- - virsh_memonly_snapshot: + virsh_memonly_snapshot:
🧹 Nitpick comments (3)
libvirt/tests/src/snapshot/virsh_memonly_snapshot.py (3)
9-15: Quote shell args to avoid breakage on paths with spaces.Safer to quote mem_file and build options predictably.
import time +import shlex from pathlib import Path
39-46: Build opts safely; log them; consider naming the snapshot.Quoting protects paths; logging helps debug. Supplying a name improves traceability and may be required in some environments.
- def _snapshot_create(): - mem_file.parent.mkdir(parents=True, exist_ok=True) - opts = f"--no-metadata --memspec file={mem_file} --live" - if cli_diskspec: - for dev in disk_targets: - opts += f" --diskspec {dev},snapshot=no" - virsh.snapshot_create_as(vm_name, options=opts, ignore_status=False, debug=True) + def _snapshot_create(): + mem_file.parent.mkdir(parents=True, exist_ok=True) + mem_path = shlex.quote(str(mem_file)) + parts = [f"--no-metadata", f"--memspec file={mem_path}", "--live"] + if cli_diskspec: + parts += [f"--diskspec {dev},snapshot=no" for dev in disk_targets] + opts = " ".join(parts) + test.log.debug(f"snapshot-create-as opts: {opts}") + snap_name = f"memonly-{int(time.time())}" + virsh.snapshot_create_as(vm_name, snap_name, options=opts, ignore_status=False, debug=True)Please confirm your virttest wrapper accepts the name positional arg and that virsh snapshot-create-as without metadata still allows/needs a name in your CI environment.
62-65: Optionally ensure full restore of config.If the VM is alive here, backup_xml.sync() updates only persistent XML. Consider stopping the VM, syncing, then (optionally) restarting to leave a clean state for following tests.
- def teardown_test(): - if backup_xml is not None: - backup_xml.sync() + def teardown_test(): + if backup_xml is not None: + was_alive = vm.is_alive() + if was_alive: + vm.destroy() + backup_xml.sync()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
libvirt/tests/cfg/snapshot/virsh_memonly_snapshot.cfg(1 hunks)libvirt/tests/src/snapshot/virsh_memonly_snapshot.py(1 hunks)
| vmx = vm_xml.VMXML.new_from_inactive_dumpxml(vm_name) | ||
| for dev in disk_targets: | ||
| disk_dict = {"target": {"dev": dev}, "snapshot": "no"} | ||
| libvirt_vmxml.modify_vm_device(vmx, "disk", disk_dict) | ||
| test.log.debug(vm_xml.VMXML.new_from_inactive_dumpxml(vm_name)) | ||
| _ensure_running() | ||
|
|
There was a problem hiding this comment.
XML changes won’t affect a running guest; restart around the edit and sync the XML.
If the VM is already alive, setting snapshot='no' in persistent XML won’t apply to the live domain. Destroy before edit, sync, then (re)start.
- if not cli_diskspec:
- vmx = vm_xml.VMXML.new_from_inactive_dumpxml(vm_name)
- for dev in disk_targets:
- disk_dict = {"target": {"dev": dev}, "snapshot": "no"}
- libvirt_vmxml.modify_vm_device(vmx, "disk", disk_dict)
- test.log.debug(vm_xml.VMXML.new_from_inactive_dumpxml(vm_name))
- _ensure_running()
+ if not cli_diskspec:
+ was_alive = vm.is_alive()
+ if was_alive:
+ vm.destroy()
+ vmx = vm_xml.VMXML.new_from_inactive_dumpxml(vm_name)
+ for dev in disk_targets:
+ disk_dict = {"target": {"dev": dev}, "snapshot": "no"}
+ libvirt_vmxml.modify_vm_device(vmx, "disk", disk_dict)
+ vmx.sync()
+ test.log.debug(vm_xml.VMXML.new_from_inactive_dumpxml(vm_name))
+ _ensure_running()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| vmx = vm_xml.VMXML.new_from_inactive_dumpxml(vm_name) | |
| for dev in disk_targets: | |
| disk_dict = {"target": {"dev": dev}, "snapshot": "no"} | |
| libvirt_vmxml.modify_vm_device(vmx, "disk", disk_dict) | |
| test.log.debug(vm_xml.VMXML.new_from_inactive_dumpxml(vm_name)) | |
| _ensure_running() | |
| if not cli_diskspec: | |
| was_alive = vm.is_alive() | |
| if was_alive: | |
| vm.destroy() | |
| vmx = vm_xml.VMXML.new_from_inactive_dumpxml(vm_name) | |
| for dev in disk_targets: | |
| disk_dict = {"target": {"dev": dev}, "snapshot": "no"} | |
| libvirt_vmxml.modify_vm_device(vmx, "disk", disk_dict) | |
| vmx.sync() | |
| test.log.debug(vm_xml.VMXML.new_from_inactive_dumpxml(vm_name)) | |
| _ensure_running() |
| - virsh_memonly_snapshot: | ||
| type = virsh_memonly_snapshot | ||
| start_vm = yes | ||
| variants: |
There was a problem hiding this comment.
Here we can directly use variants scenario: , then no need to set up scenario separately.
There was a problem hiding this comment.
Maybe I didn't make myself clear. I mean we can use the following snippet:
variant scenario:
- xml_diskspec:
- cli_diskspec:
Then we don't add scenario = under every scenario.
| from virttest.utils_libvirt import libvirt_vmxml, libvirt_disk | ||
|
|
||
|
|
||
| def run(test, params, env): |
There was a problem hiding this comment.
It's better to have docstring for every function. You can refer to all our other scripts. Both of them have the docstring and this can help us understand this script better.
BulaYoungR
force-pushed
the
snapshot_memonly
branch
from
757114a to
a9e7c27
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
libvirt/tests/src/snapshot/virsh_memonly_snapshot.py (2)
57-61: Consider consolidating small utility functions.Past reviews suggested that many functions are too small in granularity and some should be consolidated. The
_ensure_runningfunction is simple but provides good readability and could be kept as is or inlined.You could inline this function if preferred:
- def _ensure_running(): - """Start the VM if it is not already running.""" - if not vm.is_alive(): - vm.start() - if not cli_diskspec: was_alive = vm.is_alive() if was_alive: vm.destroy() vmx = vm_xml.VMXML.new_from_inactive_dumpxml(vm_name) for dev in disk_targets: disk_dict = {"target": {"dev": dev}, "snapshot": "no"} libvirt_vmxml.modify_vm_device(vmx, "disk", disk_dict) vmx.sync() test.log.debug(vm_xml.VMXML.new_from_inactive_dumpxml(vm_name)) - _ensure_running() + if not vm.is_alive(): + vm.start()
76-78: Add meaningful key point logs for debugging.Past reviews emphasized adding key point logs for debugging, specifically noting that after VM XML updates, logs should confirm the changes took effect. The current logging looks adequate but could be enhanced.
Apply this diff to add more detailed logging:
test.log.debug(f"scenario: {case}") - test.log.debug(f"mem_file: {mem_file}") - test.log.debug(f"disk_targets: {disk_targets}") + test.log.info(f"Memory snapshot file: {mem_file}") + test.log.info(f"Disk targets to disable snapshot: {disk_targets}") + test.log.info(f"Running scenario: {case}")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
libvirt/tests/cfg/snapshot/virsh_memonly_snapshot.cfg(1 hunks)libvirt/tests/src/snapshot/virsh_memonly_snapshot.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- libvirt/tests/cfg/snapshot/virsh_memonly_snapshot.cfg
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-24T08:01:27.874Z
Learnt from: hholoubk
PR: autotest/tp-libvirt#6579
File: libvirt/tests/src/sriov/vIOMMU/iommu_device_lifecycle.py:95-97
Timestamp: 2025-09-24T08:01:27.874Z
Learning: In the libvirt test framework used in tp-libvirt, VM cleanup including destroying running VMs is handled by the teardown method (test_obj.teardown_iommu_test()) called in the finally block, so explicit VM destroy calls on timeout are not necessary according to the maintainers.
Applied to files:
libvirt/tests/src/snapshot/virsh_memonly_snapshot.py
🪛 Ruff (0.13.1)
libvirt/tests/src/snapshot/virsh_memonly_snapshot.py
72-72: Expected an indented block after function definition
(invalid-syntax)
73-73: Unexpected indentation
(invalid-syntax)
88-88: Expected an indented block after function definition
(invalid-syntax)
89-89: Unexpected indentation
(invalid-syntax)
92-92: Expected a statement
(invalid-syntax)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Python 3.11
- GitHub Check: Python 3.12
- GitHub Check: Python 3.8
- GitHub Check: Python 3.9
🔇 Additional comments (6)
libvirt/tests/src/snapshot/virsh_memonly_snapshot.py (6)
26-39: Add comprehensive docstring as suggested by past reviewer.The past review comments recommended adding docstrings to improve code understanding. The current docstring is good but could be more comprehensive about the test logic and dependencies.
Apply this diff to enhance the docstring:
def run(test, params, env): """ - Entry point for Avocado-VT. + Entry point for Avocado-VT memory-only snapshot test. + + Creates a live memory-only snapshot of a running VM using virsh snapshot-create-as + with --no-metadata and --memspec options. Supports two scenarios for disabling + disk snapshots: + + 1. xml_diskspec: Modifies VM XML to set snapshot='no' for all disk devices + 2. cli_diskspec: Uses --diskspec CLI arguments to disable snapshots per disk + + Dependencies: + - Requires libvirt_disk.all_disk_targets from avocado-vt PR 4231 + - VM must be running during snapshot creation Params (from .cfg): main_vm: VM name under test (required) scenario: 'xml_diskspec' (default) or 'cli_diskspec' mem_file: Optional path to store memory dump created by --memspec - - The test ensures the VM is running, applies disk snapshot=no either via - XML or CLI, then runs: - virsh snapshot-create-as --no-metadata --memspec file=<mem> --live - Finally, it restores the original domain XML. + + Test Flow: + 1. Backup original VM XML configuration + 2. Collect all disk targets from VM + 3. Apply disk snapshot disabling (XML or CLI based on scenario) + 4. Ensure VM is running + 5. Create memory-only snapshot + 6. Restore original XML configuration """
44-46: Use recommended temporary directory location.Based on past review feedback, using
/tmpis not recommended. The code should usedata_dir.get_tmp_dir()for temporary files, which is already implemented correctly here.
92-96: VM cleanup is handled by framework teardown.Based on the retrieved learnings, VM cleanup including destroying running VMs is handled by the teardown method in the libvirt test framework, so explicit VM destroy calls are not necessary. The current approach using
finallyfor XML restoration is appropriate.
75-75: Cache disk targets to avoid duplicate calls.Past review comments noted that
_all_disk_targets()(nowall_disk_targets) is invoked more than once and suggested defining a variable to reference it in different functions. The current code calls it only once and stores it indisk_targets, which is good practice.
80-85: Potential VM state issue: XML changes may not apply to a running VM.Similar to the concern raised in previous reviews, modifying VM XML while the VM is running may not take effect immediately. The current implementation modifies the persistent XML but doesn't ensure the changes apply to the live VM.
Apply this diff to ensure XML changes are applied properly:
if not cli_diskspec: + was_alive = vm.is_alive() + if was_alive: + vm.destroy() vmx = vm_xml.VMXML.new_from_inactive_dumpxml(vm_name) for dev in disk_targets: disk_dict = {"target": {"dev": dev}, "snapshot": "no"} libvirt_vmxml.modify_vm_device(vmx, "disk", disk_dict) + vmx.sync() test.log.debug(vm_xml.VMXML.new_from_inactive_dumpxml(vm_name))
23-23: Ensureall_disk_targetsis available
libvirt/tests/src/snapshot/virsh_memonly_snapshot.py:75 callslibvirt_disk.all_disk_targets, which isn’t defined in this repo. Verify your avocado-vt installation includes PR 4231 (adding this function) or vend a fallback implementation.
BulaYoungR
force-pushed
the
snapshot_memonly
branch
2 times, most recently
from
23fef18 to
fc775df
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
libvirt/tests/src/snapshot/virsh_memonly_snapshot.py (2)
62-70: Assert memspec output exists (stronger test signal).After snapshot, verify the memory file exists and is non‑empty.
Apply this diff:
virsh.snapshot_create_as(vm_name, options=opts, ignore_status=False, debug=True) + # Ensure memory-only snapshot produced a file + if not mem_file.exists() or mem_file.stat().st_size == 0: + test.fail(f"memspec file not created or empty: {mem_file}")If you adopt
extra=, make the same change on that line accordingly.
47-48: Use explicit default and clearer naming for scenario.Make default explicit and prefer ‘scenario’ over ‘case’ for readability.
Apply this diff:
- case = params.get("scenario", "") - cli_diskspec = case == "cli_diskspec" + scenario = params.get("scenario", "xml_diskspec") + cli_diskspec = scenario == "cli_diskspec" @@ - test.log.debug(f"scenario: {case}") + test.log.debug(f"scenario: {scenario}")Also applies to: 76-76
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
libvirt/tests/cfg/snapshot/virsh_memonly_snapshot.cfg(1 hunks)libvirt/tests/src/snapshot/virsh_memonly_snapshot.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- libvirt/tests/cfg/snapshot/virsh_memonly_snapshot.cfg
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-24T08:01:27.874Z
Learnt from: hholoubk
PR: autotest/tp-libvirt#6579
File: libvirt/tests/src/sriov/vIOMMU/iommu_device_lifecycle.py:95-97
Timestamp: 2025-09-24T08:01:27.874Z
Learning: In the libvirt test framework used in tp-libvirt, VM cleanup including destroying running VMs is handled by the teardown method (test_obj.teardown_iommu_test()) called in the finally block, so explicit VM destroy calls on timeout are not necessary according to the maintainers.
Applied to files:
libvirt/tests/src/snapshot/virsh_memonly_snapshot.py
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Python 3.9
- GitHub Check: Python 3.11
- GitHub Check: Python 3.8
🔇 Additional comments (6)
libvirt/tests/src/snapshot/virsh_memonly_snapshot.py (6)
44-46: Good: use Avocado data_dir tmp for mem file.Aligns with cleanup best practices; avoids /tmp.
84-85: Thanks for the post-edit XML debug log.Matches prior feedback; helpful for diagnosing XML mutations.
87-91: Restore of persistent XML via backup_xml.sync() looks fine.Safe to restore persistent config; live config stays untouched during test.
If live XML was modified elsewhere, confirm persistent restore is sufficient for subsequent runs.
75-76: Verify libvirt_disk.all_disk_targets availability in test environment
Relies on avocado-vt PR 4231 introducing this function; confirm the updated avocado-vt package (with all_disk_targets) is installed before merging.
69-69: Verifysnapshot_create_asparameter: confirm whether virttest’s wrapper expectsextra=rather thanoptions=and update the call accordingly.
79-85: Make XML edits effective on the live domain (destroy, sync, restart).Edits to inactive XML don’t affect a running guest. Destroy before edit, sync, then (re)start to ensure snapshot=no is honored for the live snapshot.
Apply this diff:
- if not cli_diskspec: - vmx = vm_xml.VMXML.new_from_inactive_dumpxml(vm_name) - for dev in disk_targets: - disk_dict = {"target": {"dev": dev}, "snapshot": "no"} - libvirt_vmxml.modify_vm_device(vmx, "disk", disk_dict) - test.log.debug(vm_xml.VMXML.new_from_inactive_dumpxml(vm_name)) - _ensure_running() + if not cli_diskspec: + was_alive = vm.is_alive() + if was_alive: + vm.destroy() + vmx = vm_xml.VMXML.new_from_inactive_dumpxml(vm_name) + for dev in disk_targets: + disk_dict = {"target": {"dev": dev}, "snapshot": "no"} + libvirt_vmxml.modify_vm_device(vmx, "disk", disk_dict) + vmx.sync() + test.log.debug(vm_xml.VMXML.new_from_inactive_dumpxml(vm_name)) + _ensure_running()
| - virsh_memonly_snapshot: | ||
| type = virsh_memonly_snapshot | ||
| start_vm = yes | ||
| variants: |
There was a problem hiding this comment.
Maybe I didn't make myself clear. I mean we can use the following snippet:
variant scenario:
- xml_diskspec:
- cli_diskspec:
Then we don't add scenario = under every scenario.
| - cli_diskspec: leave XML intact, pass --diskspec <dev>,snapshot=no via CLI | ||
|
|
||
| If 'mem_file' is not provided, the test will create a unique path in tmp. | ||
| """ |
There was a problem hiding this comment.
@dzhengfy The docstrings are different with our current tp-libvirt scripts. Do you think this needs to be consistent as well? Or does his future code make it consistent?
Create a memroy only snapshot on VM. SC1: Create memory only snapshot by --diskspec snapshot=no option SC2: Specify snapshot=no in guest xml and create memory only snapshot Committer: Bolatbek Issakh <[email protected]>
BulaYoungR
force-pushed
the
snapshot_memonly
branch
from
fc775df to
62108d0
Compare
|
@BulaYoungR Please attach your test results which is requied for each PR. |
|
@BulaYoungR As this PR has been open for over 3 months and most comments have been updated, I would like merge it now. Please keep monitor the test result in CI job and fix any failure if any. Thanks. |
3 participants
Create a memroy only snapshot on VM.
SC1: Create memory only snapshot by --diskspec snapshot=no option
SC2: Specify snapshot=no in guest xml and create memory only snapshot
Committer: Bolatbek Issakh [email protected]
Summary by CodeRabbit